You should also confirm that get_demo_files(tmpdirname) fails here too (since there is a file but it gets filtered out).

I'm a little worried that this will eat up a lot of memory for large demo files, since it will decompress everything at once (in the original issue, demos were 1GB and that includes compressed visual observations). I think a better approach would be something like (very rough sketch):

current_brain_info = BrainInfo.from_agent_proto(0, [pair_infos[0].agent_info], brain_params)  # TODO handle empty pair_infos
for idx, current_pair_info in enumerate(pair_infos):
  # (dont calculate current_brain_info)
  # (rest of the loop stays the same)
  current_brain_info = next_brain_info

Does that sounds reasonable?

Yes, great suggestion! This way, the entire demonstration file doesn't need to be in memory at once. I want to write a test for the demonstration loading that verifies that the loading is still being done correctly. My idea is to take the old implementation, load the demo files in test_demo_dir with demo_to_buffer and then pickle the returned objects and save them somewhere as the "correct answer." Then, in test_demo_loader.py, load in the pickled objects and compare them against what the current implementation of demo_to_buffer is returning.

The problem is that the AgentBuffer and BrainParameters classes don't have defined __eq__ methods yet. Any ideas? Should I go through with the test or not? If so, what should my next step be?

This way, the entire demonstration file doesn't need to be in memory at once.

The way I proposed still has the protobuf objects loaded all at once (this is the current behavior) but it at least doesn't increase the peak. Adding things to the buffer as soon as they're read from the file is another option, but could probably wait until another PR.

Instead of saving pickled data, how about writing new demonstration files and comparing the values? We don't currently have a python implementation of this, but I think it would be useful to have anyway.

You can see how this is done in an old gist that I wrote: https://gist.github.com/chriselion/3714d05255eea2f9132b96a182fbdcaa#file-convert_demo-py-L101-L115
(the structures being written might have changed a bit, but the overall format is the same).

So the test would be something like

1. Create new BrainParameters and AgentInfo protos with specific values
2. Save as a new demonstration to a temp directory (or temp file)
3. Load and compare to the original values

Adding __eq__ to BrainParameters sounds fine if you think it would help. I'm not sure it's necessary for AgentBuffer since that inherits from dict (whether or not it should is another discussion).

I don't think I quite get what you mean. Every iteration through the for loop (in your proposed implementation), don't we lose an object reference? In particular, the object that current_brain_info referred to before current_brain_info and next_brain_info got updated. After the reference to that object is lost, wouldn't Python garbage collect that data, and, so not all the demonstration file is in memory at once?

And, just so I'm clear, you're proposing to reconvert the loaded demonstration file contents back into a demonstration file (in a temporary directory) and then directly compare the file contents? I was just thinking about if we wanted to have tests for different sequence_lengths for the make_demo_buffer function. Wouldn't this approach only work for sequence_length = 1?

Sorry for the delay.

By "still has the protobuf objects loaded all at once", I meant the pair_infos list. If we wanted to reduce the total memory, we could combine load_demonstration and make_demo_buffer so that the make_demo_buffer() loop logic happens right after agent_info_action.ParseFromString(). But I don't think we should worry about this for now; I just don't want to increase the peak usage, but we don't need to try to decrease it.

For a test, I'm suggesting something like

1. Make temporary directory
2. Create BrainParameters and AgentInfos
3. Write demo file using BrainParameters and AgentInfos to temp directory
4. Run load_demonstration() and make_demo_buffer() to get the a buffer
5. Make sure the result from 4 agrees with what should be produced from 2 (I'm not exactly sure what this looks like off the top of my head, and not quite sure about the sequence length)

A logistical note - there are some more changes to this surrounding code in the next week or so. To minimize the number of merge conflicts you're going to have to deal with, let's try to get the get_demo_files() and load_demonstration() changed merged, and come back to the make_demo_buffer / proto conversion change after.

Can you either undo the changes to make_demo_buffer(), or move them to another PR and I'll approve that? (the former is probably easier)

@chriselion Sorry for the delay. School just started for me and it took a while to get myself settled in! I'll undo the changes now so that you can merge the get_demo_files() and load_demonstration() changes!

No worries. Let me know if you want to keep working on this, or want to try something else.

@chriselion I really want to complete this first assignment, and I'll try to do so by the end of this week! But, that being said, I'd really appreciate it if you could start to onboard me to the next thing I could be working on at the same time. Much appreciated!